-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Sanitize auto-generated output tool name to support generic types #2979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@lionpeloux Thanks Lionel! Can you please add a test? |
|
This PR is stale, and will be closed in 3 days if no reply is received. |
|
I’ll get back to it next week hopefully. |
|
This PR is stale, and will be closed in 3 days if no reply is received. |
Fix AppliedI've identified and fixed the bug in the tool name sanitization regex. The IssueThe original regex pattern
This caused all the test failures you were seeing. The FixChanged the regex pattern to Testing
The PR should now pass all CI checks! |
Co-authored-by: Copilot <[email protected]>
…ctured output streaming (pydantic#3032)
…ndor_metadata` (pydantic#2987) Co-authored-by: Douwe Maan <[email protected]>
…t_stream_handler=...)` (pydantic#3084)
The previous regex pattern `[^a-z0-9-_]` was removing uppercase letters from class names, causing test failures. For example, "Foo" became "oo". This fix changes the pattern to `[^a-zA-Z0-9-_]` to preserve both uppercase and lowercase letters while still removing invalid characters like brackets from generic type names (e.g., `Result[StringData]`). Also adds a test case to verify that generic class names with brackets are properly sanitized while preserving valid characters. Fixes test failures in test_response_multiple_return_tools and related tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
74d15fb to
b445f0b
Compare
PR Branch Updated with Latest MainI've successfully rebased the PR branch onto the latest
What ChangedThe PR now includes:
Testing StatusAll tests pass locally after the rebase, confirming that the fix is compatible with the latest codebase changes. The CI should now run against the updated branch and all tests should pass! 🚀 |
Resolved conflicts by: - Accepting all incoming changes from main - Preserving the tool name sanitization fix in _output.py - Re-adding the test_output_type_generic_class_name_sanitization test The PR now includes: - Tool name sanitization with uppercase letter support [^a-zA-Z0-9-_] - Comprehensive test for generic type bracket removal - All latest changes from main (Prefect support, image generation, etc.)
|
@DouweM I've added the test. Let me know it something goes wrong. |
tests/test_agent.py
Outdated
| assert len(m.last_model_request_parameters.output_tools) == 2 | ||
|
|
||
| # Check that tool names don't contain brackets | ||
| tool_names = [tool.name for tool in m.last_model_request_parameters.output_tools] | ||
| for tool_name in tool_names: | ||
| assert '[' not in tool_name, f"Tool name '{tool_name}' contains brackets" | ||
| assert ']' not in tool_name, f"Tool name '{tool_name}' contains brackets" | ||
| # Verify the name follows the pattern [a-zA-Z0-9_-] | ||
| assert re.match(r'^[a-zA-Z0-9_-]+$', tool_name), f"Tool name '{tool_name}' contains invalid characters" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to add this (the snapshot will be filled in automatically when you run the test so that we see the names we ended up with.
| assert len(m.last_model_request_parameters.output_tools) == 2 | |
| # Check that tool names don't contain brackets | |
| tool_names = [tool.name for tool in m.last_model_request_parameters.output_tools] | |
| for tool_name in tool_names: | |
| assert '[' not in tool_name, f"Tool name '{tool_name}' contains brackets" | |
| assert ']' not in tool_name, f"Tool name '{tool_name}' contains brackets" | |
| # Verify the name follows the pattern [a-zA-Z0-9_-] | |
| assert re.match(r'^[a-zA-Z0-9_-]+$', tool_name), f"Tool name '{tool_name}' contains invalid characters" | |
| tool_names = [tool.name for tool in m.last_model_request_parameters.output_tools] | |
| assert tool_names == snapshot() |
|
This PR is stale, and will be closed in 3 days if no reply is received. |
From pydantic#2929 We ensure that auto-generated tool output names are properly sanitized so they conform to `[a-zA-Z0-9-_]`. All characters not in this pattern will simply be ignored/skipped in the final name. The error was first remarked when using generic classes as `output_type`, where brackets in the tool name would be rejected by the provider API. Added snapshot test to verify the sanitized tool names are generated correctly from generic types like Result[StringData] and Result[int]. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
30e9921 to
a854960
Compare
|
Hi @DouweM, I've merged your suggestion. Thanks for your review. |
tests/test_agent.py
Outdated
|
|
||
| def test_output_type_generic_class_name_sanitization(create_module: Callable[[str], Any]): | ||
| """Test that generic class names with brackets are properly sanitized.""" | ||
| module_code = ''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can inline this as regular code -- we're only using a stringified module above because we're injecting code {union_code}.
As suggested by @DouweM, the test now defines classes directly inline instead of using the create_module helper with a string, since we're not dynamically generating code. Note: The snapshot changed because inline classes have their function scope in the qualified name, resulting in a longer sanitized tool name. This still correctly tests bracket sanitization. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
@DouweM I've inlined the code as you suggested. Note about the snapshot change: The tool name changed from The sanitizer processes this qualified name and strips out the dots and special characters, resulting in the longer name. This is correct behavior and still properly tests that brackets from generic types like Let me know if you'd prefer to keep the |
|
@lionpeloux Ah yeah that's a bit confusing. I'm OK with defining the classes at the top level of the file, I'd rather have that than a string module. |
|
@DouweM Done! I've moved the classes to module level as you suggested. Changes:
This gives us the best of both worlds - readable inline code (no string modules) and clean snapshot names (no function scope pollution). Test passes successfully! |
|
@lionpeloux Don't forget to push :) |
As suggested by @DouweM, moved ResultGeneric and StringData classes to the top of the test file (after Person class) instead of defining them inline within the test function. This approach: - Avoids the string module approach (create_module) - Provides clean tool names in snapshots without function scope - Follows the existing pattern in the test file Tool names are now clean: final_result_ResultGenericStringData and final_result_ResultGenericint. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
@DouweM Classes are now at the TOP of the file! I moved The test passes successfully with clean tool names in the snapshot. |
|
@lionpeloux Thanks Lionel! |
From #2929
We ensure that auto-generated tool ouput name are properly sanitazed so they conform to
[a-z0-9-_].All char not in this pattern will simply be ignored/skipped in the final name.
The error was first remarked when using generic classes as
output_type, where brakets in the tool name would be rejected by the provider API.